@sebmarkbage @zpao This opens the door for some things I want to try and also should make reconciliation a little more robust because we don't move around the wrong nodes if extra nodes are in our DOM. (Whether we actually want to support that is another question.) My way here of "returning" removedChildren from ReactChildReconciler.unmountChildren makes me sad but it's not so different from what the same function is already doing with its previous return value nextChildren. I admit I don't fully understand what the plan is/was with splitting ReactChildReconciler from ReactMultiChild but hopefully this stays within the bounds of reason. It does use an extra hash map but DOMChildrenOperations loses its own temporary map and array, as well as a bunch of complexity.

Seem okay?

(Forgot to mention: the indices aren't derivable from the mount image or even the instance (since they get mutated around the same time), so we do need to store both here if other renderers want to use the indices.)

The overall goal of the ReactChildReconciler was to try to extract any recursion from the mixin. So that recursion always happened in the same predictable way regardless of output mode.

I could also pass removedChildrenRef to ReactChildReconciler.updateChildren as a reused empty [undefined] "ref cell" and mutate its contents from within that method so that the {} object is allocated only when there are removals. Getting grosser, but not really more complex.

Do you think this removedChildren object is necessary to the algorithm or an artifact of the architecture/layout of files/functions?

I think if the instantiation and destruction are done in a pass before the actual backend operations are decided (as is done currently), there needs to be some way to return information about which nodes were destroyed/unmounted, as you can't get any information off of an unmounted node. Here this is an actual DOM node; on React Native it would probably be the ID of a native node.

If ReactChildReconciler calls out to the renderer while looping through the children, then it is probably not necessary. So I guess: I think removedChildren is as necessary as the return value of nextChildren is. Probably both could be removed if we did things differently. Making updateChildren() more general like traverseAllChildren() instead of always returning a keyed object like it and flattenChildren() currently do.

Now I remember. The reason I experimented with a return value for nextChildren was because there are renderers or parts of renderers where moves are not needed. You only need to replace the old children with the new set.

The ideal data structure mutation, if available, is:

node.children = newChildren;

Or in something like gl where order doesn't matter at all.

That's why I didn't want to require the core ReactChildReconciler to do the reordering work.

Sooo what do you want me to do here?

One question: Why does ReactMultiChild call mountComponent but ReactChildReconciler calls unmount? If they both happened in ReactMultiChild that would also solve my problem; if they both happen in ReactChildReconciler then you need to return both the internal instances and the "mount images" needed by the native backend.

That is the missing piece that was left unimplemented. ReactChildReconciler should call mount too. The idea was that instantiate and mount should be the same phase and happen at the same time. For some reason I couldn't or it was just difficult. But, if you could fix that, great!

How does that help you though? Not sure I understand how that adds up.

If instantiate and mount happen together, the "mount images" (HTML string or DOM node) need to be returned from ReactChildReconciler. I assume the actual instances need to be stored too, so both need to be returned. Likewise, when unmounting, we need to pass back a backend node/reference/ID.

I was saying that ReactMultiChild calling unmountComponent would help me because then I could call getNativeNode, then unmountComponent, then enqueue the unmount operation. Sounds like you want to do the opposite, which is also fine.

Maybe we should just bite the bullet and make the multi-child diffing logic a compiler flag that other renderers can disable? I don't see a way to do this without a smarter compiler that can eliminate the allocation costs of the queue.

Perhaps a generator could allow control flow to jump between the two code paths.

Maybe we should just bite the bullet and make the multi-child diffing logic a compiler flag that other renderers can disable?

I don't know what you mean by "multi-child diffing logic".

I don't see a way to do this without a smarter compiler

Do what, sorry?

that can eliminate the allocation costs of the queue.

Are you counting my removedChildren object as "the queue"? I'm not otherwise changing how the mutations are queued in ReactMultiChild.

Perhaps a generator could allow control flow to jump between the two code paths.

You mean, ReactChildReconciler.updateChildren would be a generator that yields each operation? That seems okay except you'd need to allocate an object for each operation it returns so I'm not sure where the win is.

Do what, sorry?

I'm just thinking about how to create this decoupling between a renderer's implementation and abstracting common behavior. We don't have zero-cost abstractions. We would need Rust or something to get that.

Are you counting my removedChildren object as "the queue"

Yes.

Is this the first time we assume that NativeNode and the mount image is the same thing? Perhaps they need to be renamed to avoid confusion. I think it is fine for the internal native node and mount image to be the same but I don't think this should be confused with the thing returned by findDOMNode and findeNodeHandle even though they happen to be in React DOM and React Native.

All of our current renderers use ReactMultiChild. It isn't currently clear to me which parts of it and ReactChildReconciler are useful for prerendering and GL so it is difficult for me to reason about this currently-hypothetical abstraction barrier that you want to keep in place. Can we just fork ReactChildReconciler into a different file when we actually need different behavior? We can unify afterwards. You always say that no abstraction is better than the wrong abstraction.

This doesn't really assume that the native node and mount image are the same thing though I may have muddied the words a little bit here. In particular, the mount image is sometimes an HTML string, even on the client (particularly, when reviving server markup). It is also actually a "DOMLazyTree" which is not the actual node in IE, but a wrapper around it. The result of getNativeNode is used by replaceNodeWithMarkup and by MOVE_EXISTING and REMOVE_NODE in DOMChildrenOperations. The mount image is used by INSERT_MARKUP and ReactMount._mountImageIntoNode, as well as ReactDOMComponent itself when creating the children.

I can rename the ones I added to "Node" or "NativeNode" since they're all that.

The problem now is that this logic is so complicated and changes frequently enough that what we end up changing the renderers to fit this logic. It's self-fulfilling. I did originally fork it so that we could do local optimizations for ART but it was remerged later - sub-optimally.

With these changes we're getting closer to a universal. I'd rather go with this as is, taking the perf hit, than merging them completely.

Let's get this in an keep iterating later. We know we want the core of this at least.

I think we should drop the indices solution completely. I don't know of any scenario/target other than innerHTML where that would be needed. Although RN probably currently relies on it, we should fix it.

Thanks.

@spicyj updated the pull request.